Generate Markdown table of tools #473

jayvdb · 2024-05-05T03:44:56Z

Closes #440

This is still a bit of a mess, and the new logic is in the wrong spot - it is currently designed to easily update the existing manifests rather than supporting creating new manifests.

Key question atm is whether it is OK to add new fields website & license_markdown to both the base info and manifest structs.

Another is whether it is OK to create a lib.rs ? I moved everything to the lib.rs , but would be happy to move the unmodified parts back to main.rs to reduce the size of the change.

taiki-e · 2024-05-05T05:04:41Z

Thanks!

Key question atm is whether it is OK to add new fields website & license_markdown to both the base info and manifest structs.

I'm okay with adding those as those are small enough information.

Another is whether it is OK to create a lib.rs ? I moved everything to the lib.rs , but would be happy to move the unmodified parts back to main.rs to reduce the size of the change.

I'm okay with either.

jayvdb · 2024-05-05T05:14:39Z

Great.

Next two questions:

Is it ok to generate as TOOLS.md , and link to it from README.md? IMO the list is already getting long. I dont mind merging it into the README.md . It is only a bit more work to do that.
Should the markdown generator be a separate tool , or run at the end of the existing main.rs ?

taiki-e · 2024-05-05T05:31:16Z

Is it ok to generate as TOOLS.md , and link to it from README.md? IMO the list is already getting long. I dont mind merging it into the README.md . It is only a bit more work to do that.

The current list is indeed long, so splitting it into separate files is okay.

Should the markdown generator be a separate tool , or run at the end of the existing main.rs ?

I think the latter is better at this time because it eliminates the need for changes to the CI script.

jayvdb · 2024-05-05T06:35:06Z

IIUC, the CI loops over each tool, in which case putting the markdown generation in the main.rs will mean it does that step many times - once per tool, instead of only once at the end. Anyways, I'll put it in main.rs, but put the logic in lib.rs, so it can become a separate bin later.

taiki-e · 2024-05-05T06:41:52Z

IIUC, the CI loops over each tool, in which case putting the markdown generation in the main.rs will mean it does that step many times - once per tool, instead of only once at the end.

Oh, you are right. That is indeed inefficient and would be better to separate them.

taiki-e · 2024-05-19T16:40:33Z

TOOLS.md

@@ -0,0 +1,47 @@
+# Tools


It seems that the new table lost entries that are not managed by manifest from the previous, such as nextest, valgrind.

I've done a comparison now, and it is only nextest and valgrind from README.md that dont appear in TOOLS.md.

nextest is easy - we can create a base manifest for it, to get the bins at https://github.com/nextest-rs/nextest/releases/tag/cargo-nextest-0.9.70

That leaves valgrind, which is special as it isnt on GitHub.

Not too hard to have a special case for valgrind.

But, I have also noticed that cargo-watch & watchexec-cli is in

install-action/tools/ci/tool-list.sh

Lines 135 to 138 in 689459d

latest) tools+=(cargo-watch watchexec-cli nextest) ;;

major.minor.patch) tools+=([email protected] [email protected] [email protected]) ;;

major.minor) tools+=([email protected] [email protected] [email protected]) ;;

major) tools+=(cargo-watch@8 watchexec-cli@1) ;;

, but these are not in README.md . Any others that you know about ?

cargo-watch can also be a base manifest, getting bins from https://github.com/watchexec/cargo-watch/releases/tag/v8.5.2

watchexec-cli might be the bins at https://github.com/watchexec/watchexec/releases/tag/v2.1.1 - need to double check that one.

I see quite a few other system tools that are installed by https://github.com/taiki-e/install-action/blob/main/main.sh . Should we be mentioning them also? jq, curl, tar also ?

They would be more special cases, which is fine.

jq could be a manifest - see https://github.com/jqlang/jq/releases/tag/jq-1.7.1

https://github.com/curl/curl/releases/tag/curl-8_7_1 doesnt contain binaries

Maybe we could be replacing tar with https://github.com/vadorovsky/packy/releases/tag/v0.0.3 , if it released more arch binaries, and it uploaded untar'd bins to avoid chicken and egg problem.

But, I have also noticed that cargo-watch & watchexec-cli is in

tools/ci/tool-list.sh is only for testing. tools/publish.sh is for "Supported tools".

install-action/tools/publish.sh

Lines 105 to 106 in 689459d

# Not manifest-based

tools+=(valgrind nextest cargo-nextest)

I see quite a few other system tools that are installed by https://github.com/taiki-e/install-action/blob/main/main.sh . Should we be mentioning them also? jq, curl, tar also ?

Let's ignore them at this PR, as they are for incomplete runners and are not usually installed by us.

tools/codegen/src/tools-markdown.rs

jayvdb · 2024-05-20T00:41:05Z

tools/codegen/src/lib.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Version {
+    pub major: Option<u64>,
+    pub minor: Option<u64>,
+    pub patch: Option<u64>,
+    pub pre: semver::Prerelease,
+    pub build: semver::BuildMetadata,
+}


fwiw, git diff --color-moved is a good way to check changes to the items which were moved to lib.rs.

Or if you like, I can create a separate PR to move them to lib.rs , but it still involves a lot of changes because pub needed to be added to the struct members

tools/codegen/src/main.rs

jayvdb · 2024-05-20T00:59:31Z

tools/codegen/src/main.rs

@@ -561,6 +636,35 @@ fn download_github(url: &str) -> Result<ureq::Response> {
    Err(last_error.unwrap().into())
 }

+#[allow(clippy::missing_panics_doc)]
+pub fn github_head(url: &str) -> Result<()> {
+    eprintln!("fetching head of {url} ..");


this & the eprintln for failure is a bit noisy, especially as the license algorithm expects this to fail quite often as it tries to detect where to find the license file.

TOOLS.md

.vscode/settings.json

TOOLS.md

taiki-e

Looks great! Thanks @jayvdb!

jayvdb force-pushed the gen-readme-table branch from 719fde3 to 8234a58 Compare May 5, 2024 03:53

jayvdb mentioned this pull request May 19, 2024

codegen: Allow skipping existing versions #485

Merged

jayvdb force-pushed the gen-readme-table branch 6 times, most recently from 953952b to 6b78595 Compare May 19, 2024 10:49

jayvdb marked this pull request as ready for review May 19, 2024 11:58

taiki-e reviewed May 19, 2024

View reviewed changes

jayvdb mentioned this pull request May 19, 2024

Creating a manifest for nextest #487

Closed